It would be great to get some feedback from @cijothomas, @MikeGoldsmith and @martinjt if they have time. I think I've populated the most important tags that are easy to get, but in case I'm missing some, it'd be great to get feedback.

Hello @stebet - would you have a moment to give an update here?

This should hopefully be ready now. Feel free to give it another pass @cijothomas if you have time :) @bollhals might find this interesting as well.

This should have minimal overhead when tracing is not enabled since no Activity generation occurs and no tags need to be populated.

Would you mind running some tests on this (with and without tracing enabled)? I suspect some boxing occurring even in the disabled case.
For the enabled case, this will cause significant impact (performance and allocation wise), as it has lots of tostrings and boxing and stuff.

One question I had was do we need a separate option to enable it? E.g. my business has tracing enabled, but we'd might not want to know this level of detail from RabbitMQ.

Would you mind running some tests on this (with and without tracing enabled)? I suspect some boxing occurring even in the disabled case. For the enabled case, this will cause significant impact (performance and allocation wise), as it has lots of tostrings and boxing and stuff.

Yeah, will do. Although even when tracing is enabled, if the activity isn't sampled, it'll still skip A LOT of the processing since it checks first if the Activity is requesting all data.

B.t.w, overhead is expected, but the APIs are very fast and used for both runtime stuff (SQL client, HTTP client) as well as as third party libraries like StackExchange.Redis so i.m.o the overhead when tracing is enabled is acceptable and known. That's why Sampling is a built-in feature for Activities.

One question I had was do we need a separate option to enable it? E.g. my business has tracing enabled, but we'd might not want to know this level of detail from RabbitMQ.

Yes. For example if you are using the OpenTelemetry libraries you'll need to do something like this:

builder.Services.AddOpenTelemetryTracing(builder =>
{
    builder
    .Configure((IServiceProvider serviceProvider, TracerProviderBuilder traceBuilder) =>
    {
        traceBuilder
        .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("test-app", serviceVersion: "1.0.0"))
        .AddSource("RabbitMQ.Client") // Enable tracing for the RabbitMQ.Client library. Not setting this causes no activities to be created within the library.
        .AddAspNetCoreInstrumentation(options => options.RecordException = true)
        .... // more initialization stuff here
        ;
    });
});

Would you mind running some tests on this (with and without tracing enabled)? I suspect some boxing occurring even in the disabled case. For the enabled case, this will cause significant impact (performance and allocation wise), as it has lots of tostrings and boxing and stuff.

Yeah, will do. Although even when tracing is enabled, if the activity isn't sampled, it'll still skip A LOT of the processing since it checks first if the Activity is requesting all data.

B.t.w, overhead is expected, but the APIs are very fast and used for both runtime stuff (SQL client, HTTP client) as well as as third party libraries like StackExchange.Redis so i.m.o the overhead when tracing is enabled is acceptable and known. That's why Sampling is a built-in feature for Activities.

Yes, sure it's expected if you enable such a feature, but there are still things we could optimize for to avoid allocations. But this can also wait for later. But we should look at overhead of the two other options of no tracing enabled and tracing enabled but not for RabbitMQ.

One question I had was do we need a separate option to enable it? E.g. my business has tracing enabled, but we'd might not want to know this level of detail from RabbitMQ.

Yes. For example if you are using the OpenTelemetry libraries you'll need to do something like this:

builder.Services.AddOpenTelemetryTracing(builder =>
{
    builder
    .Configure((IServiceProvider serviceProvider, TracerProviderBuilder traceBuilder) =>
    {
        traceBuilder
        .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("test-app", serviceVersion: "1.0.0"))
        .AddSource("RabbitMQ.Client") // Enable tracing for the RabbitMQ.Client library. Not setting this causes no activities to be created within the library.
        .AddAspNetCoreInstrumentation(options => options.RecordException = true)
        .... // more initialization stuff here
        ;
    });
});

Ah yes, forgot about it, as I only had to do it once for our project :D Thanks for reminding me.

@bollhals
I did some simple benchmarks with the MassTest app modified to do 1000 batches (up from 100) with 500 items each, and seems the overhead is minimal (after I fixed a small bug):

Current master build

ActivitySource build with OpenTelemetry initialized but not listening for RabbitMQ.Client events

Hi @stebet, thanks for making this PR. Do you think we could have it in the next coming months?

Rebased this on the latest master and removed the need for OpenTelemetry.API reference as suggested by @rwkarg :)

Updated to be in line with current spec. Thanks @Kielek, would love it if you could verify that I got things correcly :)

If people need this feature soon, a user has found the following workaround that uses AMQP headers:

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/examples/MicroserviceExample

@cijothomas I personally know next to nothing about OpenTelemetry and the changes in this PR, so someone who is more familiar will have to address your concerns:

• Adding examples/docs. The name of the ActivitySource used must be documented so OTel consumers can enable this instrumentation
• Call out that this relies on experimental semantic convention and is subject to change, so users are not caught with surprise if the conventions change before they stabilize.

@stebet I'm merging the latest main into this PR at the moment.

@stebet I have completed rebasing this PR on main ... please go over the code again to make sure I didn't change something inadertently.

@stebet I have completed rebasing this PR on main ... please go over the code again to make sure I didn't change something inadertently.

I'll take a look in a bit.

@stebet everything seems to be fine, except that the Test.Integration.TestActivitySource.TestPublisherAndBasicGetActivityTags is flaky on GitHub actions - https://github.com/rabbitmq/rabbitmq-dotnet-client/actions/runs/7658052226/job/20870377811

Of course it works every time on my machine.

@stebet everything seems to be fine, except that the Test.Integration.TestActivitySource.TestPublisherAndBasicGetActivityTags is flaky on GitHub actions - https://github.com/rabbitmq/rabbitmq-dotnet-client/actions/runs/7658052226/job/20870377811

Of course it works every time on my machine.

I'll take a look now and see if I see anything obvious

@lukebakken Moved the Activity tests over to the sequential tests since they are manipulating static variables.

Glad to see this making progress!

Will do.

• Documentation/examples is handled in another repo AFAIK - @stebet are you referring to the repo that is used to create this web site?

Yes.

@cijothomas I would appreciate example code and doc text that I can use to update the web site. Thanks.

#1478

Excellent! I think we'd like to get this backported to the older version. Before we do that though, is it possible to get a preview build that we can ship with an aspire preview so we can get some mileage on it?